Implement SVCB #1067

DesWurstes · 2019-12-30T07:39:41Z

Resolves #1064

Seems to work well. Can be tested with @ghedo's python code and his Go code (Now my code uses the same numbers. I've updated his Go code, below, to work with the new edns-like code:)

package main

import (
	"fmt"
	"github.com/DesWurstes/dns"
	"net"
	"sync"
)

func handleRequest(w dns.ResponseWriter, r *dns.Msg) {
	fmt.Println("GOT REQUEST", r)

	m := new(dns.Msg)
	m.SetReply(r)

	svcb := &dns.SVCB{
		Hdr: dns.RR_Header{
			Name:   r.Question[0].Name,
			Rrtype: dns.TypeSVCB,
			Class:  dns.ClassINET,
			Ttl:    3600,
		},
		Priority: 1,
		Target:   ".",
		Value: []dns.SVCBKeyValue{
			&dns.SVCBAlpn{
				Alpn: []string{"h2", "h3"},
			},
			&dns.SVCBPort{
				Port: 117,
			},
			&dns.SVCBIPv4Hint{
				Hint: []net.IP{net.IPv4(1, 1, 1, 1), net.IPv4(1, 1, 1, 2)},
			},
			&dns.SVCBECHConfig{
				ECH: []byte{1, 2},
			},
		},
	}

	m.Answer = append(m.Answer, svcb)
	err := w.WriteMsg(m)
	if err != nil {
		fmt.Println(err)
	}
}

func main() {
	var wg sync.WaitGroup
	wg.Add(1)

	dns.HandleFunc("svcb.example.", handleRequest)

	server := &dns.Server{Addr: ":53", Net: "udp"}
	go server.ListenAndServe()

	wg.Wait()
}

codecov-io · 2019-12-30T07:45:33Z

Codecov Report

Merging #1067 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1067   +/-   ##
=======================================
  Coverage   56.09%   56.09%           
=======================================
  Files          42       42           
  Lines       10612    10612           
=======================================
  Hits         5953     5953           
  Misses       3609     3609           
  Partials     1050     1050

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6233703...6233703. Read the comment docs.

scan_rr.go

types.go

miekg · 2019-12-30T14:14:46Z

[ Quoting <[email protected]> in "Re: [miekg/dns] Implement SVCB (#10..." ]

They're of the form `a=b` or `a="b"`. Because the values of key=value pairs consist of > basic-visible [which] is VCHAR minus DQUOTE, ";", and "\" So I should add code to remove the double quotes if they exist.

dunno, if the draft is unclear on that it should be more clear. Or send an email to dnsops to ask for clarification.

miekg · 2019-12-30T14:18:37Z

[ Quoting <[email protected]> in "Re: [miekg/dns] Implement SVCB (#10..." ]

Which means the wire format of values of key=value pairs depend on keys, and key- value-wireformat-type table was not written which means no one can implement wire format encoding/decoding before the spec is ready.

Missed this previously: https://tools.ietf.org/html/draft-ietf-dnsop-svcb-httpssvc-01#appendix-C.3 C.3. Wire Format Advice from experts in DNS wire format best practices would be greatly appreciated to refine the proposed details, overall. currently the format is: When SvcFieldValue is non-empty (ServiceForm), it contains a list of SvcParamKey=SvcParamValue pairs with length-prefixes for the SvcParamValues, each of which contains: o a 2 octet field containing the SvcParamKey as an integer in network byte order. o a 2 octet field containing the length of the SvcParamValue as an integer between 0 and 65535 in network byte order (but constrained by the RDATA and DNS message sizes). o an octet string of the length defined by the previous field. this is def *not* how the TXT record does it and a bit bonkers if you ask me.

DesWurstes · 2019-12-30T14:21:32Z

Thank you, that helps, I'll add that.

miekg · 2019-12-30T14:27:31Z

[ Quoting <[email protected]> in "Re: [miekg/dns] Implement SVCB (#10..." ]

Thank you, that helps, I'll add that.

also see #1064 (comment)

DesWurstes · 2019-12-31T16:49:56Z

Hello again, I'm adding key=value wire encoding/decoding. In #1064 you suggested those key=value pairs should be []string in the struct. I believe []byte (or even a separated [][]byte) in the wireformat would suit better, because it'd allow the library to be used as a proxy with reserved and private key types (otherwise we can't deserialize the value, which is of an unknown format). In addition, packing and unpacking would be more expensive, which I think is used more often than string serialization/deserialization. Do you agree, should the struct keep Values as binary?

miekg · 2019-12-31T17:51:28Z

I think we should help shape the wire format for this record using the experience we gather from trying to implement it. IOW engage on the dnsop ML to propose a new format

…

On Tue, 31 Dec 2019, 17:49 DesWurstes, ***@***.***> wrote: Hello again, I'm adding key=value wire encoding/decoding. In #1064 <#1064> you suggested those key=value pairs should be []string in the struct. I believe []byte (or even a separated [][]byte) in the wireformat would suit better, because it'd allow the library to be used as a proxy with reserved and private key types (otherwise we can't deserialize the value, which is of an unknown format). In addition, packing and unpacking would be more expensive, which I think is used more often than string serialization/deserialization. Do you agree, should the struct keep Values as binary? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1067?email_source=notifications&email_token=AACWIW57W2YCSFZNIVESIILQ3NZ3LA5CNFSM4KBJ2Z2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH4NZRY#issuecomment-569957575>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWIW7GCKMWPAUOWLTWEHTQ3NZ3LANCNFSM4KBJ2Z2A> .

ghedo · 2020-04-02T10:40:39Z

Hello @DesWurstes, are you still working on this by any chance? I'm interested in helping out (if any help is needed) to get this in shape (if you are not interested in working on this anymore I'm also available to take over the work and get it ready, if that's ok with you).

We (Cloudflare) are looking into shipping support for HTTSSVC, so this would help greatly.

Implements for draft-ietf-tls-esni-05, thus if we don't declare it private people will try to use with Cloudflare's servers, which are draft-ietf-tls-esni-02 that is incompatible with the SVCB record.

I don't understand this part. Why does it need to be "private"? If people try to query SVCB or HTTPSSVC records on Cloudflare's DNS servers they will get back nothing, as we don't currently serve these records. In that case a client wouldn't use an ESNI version that Cloudflare servers don't support, and it could either fallback to an older version if it supports it, or not do ESNI at all.

DesWurstes · 2020-04-02T13:11:12Z

Hello,

My implementation was incomplete because I wasn't sure if the specification would undergo major changes. It seems it has matured. Now I'll rebase it and start working on it again.

I'll change

Value    []string `dns:"txt"`

to

Value    []uint16 `dns:"svcb"`

so that it doesn't decode when not needed. (and define the type)

I wanted to make it private as the draft is not finalized so no server supports it, and those implementing SNI encryption could try to use the SVCB record instead of _sni.

DesWurstes · 2020-04-03T13:26:48Z

@miekg should I make Value field closer to the wireformat or the presentation format? (I prefer wireformat even though other ones seem to be keeping data in base64 strings instead of byte arrays but SvcValueField can be containing a key whose String name we don't know because it's reserved, thus it'd be nonconvertible to presentation format, but we shouldn't need to process SvcValueField unless .String(), to keep reserved keys when serializing to wireformat again?)

miekg · 2020-04-05T17:54:42Z

[ Quoting <[email protected]> in "Re: [miekg/dns] Implement SVCB (#10..." ]

@miekg should I make Value field closer to the wireformat than the presentation format? (Because other ones seem to be keeping data in base64 strings instead of byte arrays but SvcValueField can be containing a key whose value type we don't know because it's reserved, so we don't need to process SvcValueField unless it's required?)

I haven't been following the latest with regards to SVCB, but the structure in go dns should contain a field that is usable. I.e. the `A` records we use `net.IP`, not `[]byte` /Miek

…

-- Miek Gieben

DesWurstes · 2020-04-09T14:02:58Z

type SvcKeyValue struct {
	SvcParamKey   uint16
	SvcParamValue string
}

where SvcParamValue is the presentation format representation is fine? Then for defined keys we can add

func (rr *SVCB) getIpv4Hint() (ip net.IP, has bool) {
	...
}

etc, because the presentation format usually is helpful, but if they need it in go data types, they can call functions similar to this. (Or functions taking []SvcKeyValue as arguments, so that it's valid for both SVCB and HTTPSSVC)

Or to solve the problem similar to edns.go? For someone needing to use SVCB the EDNS approach could look too complex?

miekg · 2020-04-10T06:30:15Z

[ Quoting <[email protected]> in "Re: [miekg/dns] Implement SVCB (#10..." ]

type SvcKeyValue struct { SvcParamKey uint16 SvcParamValue string } where SvcParamValue is the presentation format representation is fine? Then for defined keys we can add func (rr *SVCB) getIpv4Hint() (ip net.IP, has bool) { ... }

Why is such a helper needed? The SvcParamKeys prolly will need a whole slew of constants defined as well...

DesWurstes · 2020-04-10T07:39:51Z

The array of SvcKeyValues would is in increasing order, they can for-loop until SvcParamKey == SVC_PORT (keys) for example. Do you prefer:

type SVCB struct {
	Hdr      RR_Header
	Priority uint16
	Target   string        `dns:"domain-name"`
	esni       ESNI `dns:"-"`
        Ipv4hint Ipv4 `dns:"-"`
        Alpn        alpn `dns:"-"`
        Nodefprotocol bool `dns:"-"`
        // other valuews
	Value    []SvcKeyValue `dns:"svc"` // if priority == 0 this is empty
}

miekg · 2020-04-10T19:22:28Z

[ Quoting <[email protected]> in "Re: [miekg/dns] Implement SVCB (#10..." ]

https://github.com/miekg/dns/pull/1067/files# diff-e871404fd69cab40521d6b2178847cf7R184-R219 The array of SvcKeyValues would be ordered, they can for-loop until SvcParamKey == SVC_PORT for example. Do you prefer: type SVCB struct { Hdr RR_Header Priority uint16 Target string `dns:"domain-name"` esni ESNI `dns:"-"` Ipv4hint Ipv4 `dns:"-"` Alpn alpn `dns:"-"` Nodefprotocol bool `dns:"-"` // other valuews Value []SvcKeyValue `dns:"svc"` // if priority == 0 this is empty }

I need to look at the current record, all those `dns:"-"` tagged values are super weird, if they are not useful to a user they should not be in the struct

DesWurstes · 2020-04-16T19:25:37Z

That was an example of solution, the specification didn't change. In other words, should we keep values in SvcFieldValue struct in the presentation format as string, with conversion functions to go types, or when decoding it should we fill, with values, go-typed spaces in our struct, which are empty when a relevant key=value pair is not found?

ghedo

Hello again,

I've tested this code against my own implementation (written in Python) from rthalley/dnspython#452 (see comment below for a bug I found). The test I did involved running a Go server that serves an SVCB record, and a Python client that queries the record. Hoepfully this can be useful during developement of this PR, so I'll leave the code I wrote below. It currently only tests the alpn parameter, but it can be easily adapted to test other features as well.

In terms of the exposed Go API, I agree that using only string for all SvcParamValues is not great, as the user will need to know how to manually encode and decode all different values to/from the wire format (which is also different from the presentation format) for most fields. This becomes repetitive as each user will need to basically re-implement the same thing over and over.

I'm not sure using hard-coded fields as suggested in #1067 (comment) would work, as all the parameters are actually optional, so it's not clear to me how a user parsing an SVCB answer would know which parameter is actually set and which isn't (maybe using nullable pointers would work better?).

Given this and the fact that each field type has different presentation and wire formats, I think implementing something like edns.go would actually be much easier for users to use. Each svc param would implement a common interface, and the user could more easily create and use the different parameter I think (and it would also be easier to add new parameters in the future).

Anyways, thanks for working on this @DesWurstes.

Here is the code for the Go server in my test:

package main

import "fmt"
import "sync"

import "github.com/DesWurstes/dns"

func handleRequest(w dns.ResponseWriter, r *dns.Msg) {
    fmt.Println("GOT REQUEST", r)

    m := new(dns.Msg)

    m.SetReply(r)

    svcb := &dns.SVCB{
        Hdr: dns.RR_Header{
            Name:   r.Question[0].Name,
            Rrtype: dns.TypeSVCB,
            Class:  dns.ClassINET,
            Ttl:    3600,
        },
        Priority: 1,
        Target:   ".",
        Value:    []dns.SvcKeyValue {
            dns.SvcKeyValue{
                SvcParamKey:   dns.SVC_ALPN,
                SvcParamValue: "\x02h3\x02h2",
            },
        },
    }

    m.Answer = append(m.Answer, svcb)

    w.WriteMsg(m)
}

func main() {
    var wg sync.WaitGroup
    wg.Add(1)

    dns.HandleFunc("svcb.example.", handleRequest)

    server := &dns.Server{Addr: ":53", Net: "udp"}
    go server.ListenAndServe()

    wg.Wait()
}

And here is the Python client:

#!/usr/bin/env python3

import dns.resolver

resolver = dns.resolver.Resolver(configure=False)
resolver.nameservers = ['127.0.0.1']

answer = resolver.query('svcb.example.', 'SVCB')

print(answer.rrset)

ghedo · 2020-04-17T16:03:38Z

msg_helpers.go

+
+func packDataSvc(options []SvcKeyValue, msg []byte, off int) (int, error) {
+	for _, el := range options {
+		off, err := packUint16(el.SvcParamKey, msg, off)


I know that this is probably going to change anyway, but note that the off variable here shadows the off argument variable, so this function always ends up returning the same value that was passed in (effectively the svc fields are not encoded in the output message).

ghedo · 2020-04-17T17:03:14Z

Oh, regarding the dnspython code, note that I accidentally ended up using different code-points there than what is used in this PR, so one or the other needs to be manually changed to make the test work.

DesWurstes · 2020-04-21T17:50:42Z

@miekg I'm ditto-ing edns.go, but I've found fields that have no use:

dns/edns.go

Lines 217 to 226 in 6737387

    
           type EDNS0_SUBNET struct { 
        
           	Code          uint16 // Always EDNS0SUBNET 
        
           	Family        uint16 // 1 for IP, 2 for IP6 
        
           	SourceNetmask uint8 
        
           	SourceScope   uint8 
        
           	Address       net.IP 
        
           } 
        
           // Option implements the EDNS0 interface. 
        
           func (e *EDNS0_SUBNET) Option() uint16 { return EDNS0SUBNET }

Code is only written to and never read (instead of .Option()). Should I not add this to SVCB (except local-experimental one)? (Are those fields only for backwards compatibility, or does that aim to remind the programmer that key is a part of the data stored too?)

miekg · 2020-04-28T09:25:49Z

svcb.go

+
+// SvcKeyValue defines a key=value pair for SvcFieldValue.
+// A SVCB RR can have multiple SvcKeyValues appended to it.
+type SvcKeyValue interface {


oh wow. I'm reading section 6 of the draft, that's pretty terrible range of things you can put in this record as key=value thingies.
Note that the EDNS0 stuff has been shown to be suboptimal and isn't the nicest interface to work with, so I rather not have it here again. Having different types implementing SvcKeyValue works for me, so this will be still an interface. However things like Key and Read look like overkill. I.e. the interface must be paired down to the absolute minimal

miekg · 2020-04-28T09:28:10Z

svcb.go

+//	e.Alpn = []string{"h2", "http/1.1"}
+//	o.Value = append(o.Value, e)
+type SVC_ALPN struct {
+	Code uint16   // Always SVCALPN


So Code isn't really need here? Maybe that can be the interface Code() uint ? Also the naming of the types should be move Go like and not uppercase with underscores (another mistake in the EDNS0 code). I'm not sure what would make sense here? A subpackage we're these things live? Or prefixing them with Svc ? Note that the final name is not chosen yet? (is this true?) So we might be looking into renaming this in the future

Rather than prefixing every member of each struct I'd prefer to move everything to a SVCB folder/subpackage. What do you think? The specification may be extended by much in the future.

I believe Key() should stay in the main interface, so that its exposed parts are

type SvcKeyValue interface { // Key returns the key code of the pair. Key() uint16 // String returns the string representation of the value. String() string }

ghedo · 2020-05-01T10:32:37Z

Just for the record, I was able to implement support for serving HTTPSSVC records in our internal DNS server after pulling these changes (including the latest API tweaks) 🎉 but I'll need to wait for this PR to be merged before starting deployment to production.

The new public API exposed to applications is quite nice, so I don't really have anything more to say 👍

DesWurstes · 2020-05-01T10:40:19Z

Hi ghedo, thanks for the feedback you've given, and your dnspython SVCB code, it was essential for me to debug this implementation. :)

miekg · 2020-05-06T12:45:15Z

[ Quoting <[email protected]> in "Re: [miekg/dns] Implement SVCB (#10..." ]

@DesWurstes commented on this pull request. ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ In svcb.go: > + +// SVC_ALPN pair is used to list supported connection protocols. +// Protocol ids can be found at: +// https://www.iana.org/assignments/tls-extensiontype-values/ +// tls-extensiontype-values.xhtml#alpn-protocol-ids +// Basic use pattern for creating an alpn option: +// +// o := new(dns.HTTPSSVC) +// o.Hdr.Name = "." +// o.Hdr.Rrtype = dns.TypeHTTPSSVC +// e := new(dns.SVC_ALPN) +// e.Code = dns.SVCALPN +// e.Alpn = []string{"h2", "http/1.1"} +// o.Value = append(o.Value, e) +type SVC_ALPN struct { + Code uint16 // Always SVCALPN Rather than prefixing every member of each struct I'd prefer to move everything to a SVCB folder/subpackage. What do you think? The specification may be extended by much in the future. I believe Key() should stay in the main interface, so that its exposed parts are type SvcKeyValue interface { // Key returns the key code of the pair. Key() uint16 // String returns the string representation of the value. String() string }

yep, this looks like a sane interface, modulo naming but we can fix that later. Yes, I'm also inclined to move this to a subpackage, `dnssvcb` or something less hidious...

miekg · 2020-05-06T12:50:11Z

[ Quoting <[email protected]> in "Re: [miekg/dns] Implement SVCB (#10..." ]

Just for the record, I was able to implement support for serving HTTPSSVC records in our internal DNS server after pulling these changes (including the latest API tweaks) 🎉 but I'll need to wait for this PR to be merged before starting deployment to production. The new public API exposed to applications is quite nice, so I don't really have anything more to say 👍

nice cross OSS/DNS work here :)

DesWurstes · 2020-05-06T15:27:43Z

I was dreaming of keeping SVCB in types.go and moving everything related to the value field to a subpackage. For encode/decode and everything this would require everything in the KeyValuePair interface (Key, String,pack, unpack, read, len) to be declared public so that we can access it from the top package. We can't have a link between the package and the subpackage that is not exposed to the user. So I'd rather not move it to a subpackage. Do you have any solution ideas? Or we can move and then expose only 4 functions which we can mark "Internal, don't use" in the documentation?

DesWurstes · 2020-07-26T10:34:08Z

@tmthrgd Thank you for the thorough review, I've addressed them all (or commented).

tmthrgd

A few changes.

duplicate_generate.go

msg_helpers.go

svcb.go

tmthrgd · 2020-08-05T08:02:47Z

svcb.go

+				fallthrough
+			case '\\':
+				str.WriteByte('\\')
+				fallthrough


What I'm saying is replace this with:

case ...: str.WriteByte('\\') str.WriteByte(e) default: str.WriteByte(e)

tmthrgd · 2020-08-05T08:08:08Z

svcb.go

+	for _, e := range s.Data {
+		if (0x1f < e && e < 0x7f) || e == 0x09 {
+			switch e {
+			case '"', ';', ' ', '\\', 0x09:


This can't be right for 0x09 (\t), because that would be \ (slash then tab).

tmthrgd · 2020-08-05T08:10:50Z

svcb.go

+	var str strings.Builder
+	str.Grow(4 * len(s.Data))
+	for _, e := range s.Data {
+		if (0x1f < e && e < 0x7f) || e == 0x09 {


Can we switch this around so it's like:

if e < ' ' || e > '~' { str.WriteString(escapeByte(e)) } else { ... }

tmthrgd · 2020-08-05T08:11:53Z

svcb.go

+// areSVCBPairArraysEqual checks if SVCBKeyValue arrays are equal
+// after sorting their copies. arrA and arrB have equal lengths,
+// otherwise zduplicate.go wouldn't call this function.
+func areSVCBPairArraysEqual(arrA []SVCBKeyValue, arrB []SVCBKeyValue) bool {


They can share the same name: a = append(...).

tmthrgd · 2020-08-05T08:12:11Z

svcb.go

+func areSVCBPairArraysEqual(arrA []SVCBKeyValue, arrB []SVCBKeyValue) bool {
+	a := append(make([]SVCBKeyValue, 0, len(arrA)), arrA...)
+	b := append(make([]SVCBKeyValue, 0, len(arrB)), arrB...)
+	sort.Slice(a, func(i, j int) bool {


My suggestion here is just to put the existing code onto one line.

DesWurstes · 2020-08-06T07:53:47Z

@tmthrgd

This can't be right for 0x09 (\t), because that would be \ (slash then tab).

Spaces and tabs are treated the same. These are allowed:

1) dnskey="a b"
2) dnskey=a\ b
3) dnskey="a\ b"

The current code uses the third form for experimental key=value pairs. It's easy to change it to the first, would you prefer the first?

They can share the same name: a = append(...).

If:

func areSVCBPairArraysEqual(a []SVCBKeyValue, b []SVCBKeyValue) bool {
	a := append([]SVCBKeyValue(nil), a...)
	b := append([]SVCBKeyValue(nil), b...)

Then:

./svcb.go:744:4: no new variables on left side of :=
./svcb.go:745:4: no new variables on left side of :=

Push 2 because can't build fuzzer python Push 3 to try again

ghedo · 2020-09-18T10:15:55Z

Looks like this has gone silent for a while, any update? It seems most review comments have been addressed, but I guess @miekg hasn't had a chance to do his "minimization" pass yet?

And add the relevant test Force push edit: Make sorting code fit into one line

tmthrgd

A handful of relatively minor changes, and then it should be ready to merge. Thanks for the patience.

msg_helpers.go

svcb.go

tmthrgd · 2020-09-27T10:16:23Z

svcb.go

+}
+
+func (s *SVCBIPv4Hint) parse(b string) error {
+	if strings.Contains(b, ":") {


I don’t love this check. Is there any harm in letting people use IPv4-in-IPv6 addresses?

tmthrgd · 2020-09-27T10:19:53Z

svcb.go

+	}
+	x := make([]net.IP, 0, len(b)/16)
+	for i := 0; i < len(b); i += 16 {
+		x = append(x, net.IP(b[i:i+16]))


This doesn’t check for IPv4 addresses, should it?

It's impossible to check for the address type, this is just a byte array.

A To4() != nil check was what I was referring to. Not sure if it’s a good idea, but it would seem consistent with other methods on this type.

Well, if it's okay for you I would prefer not adding conditions that would always evaluate to one branch, To4() is always nil for 16-byte IP arrays.

That seems inconsistent with other methods on this type that all have a To4() != nil check on them. It’s definitely not going to be a hotspot.

OK, changed it.

tmthrgd · 2020-09-27T10:20:55Z

svcb.go

+}
+
+func (s *SVCBIPv6Hint) parse(b string) error {
+	if strings.Contains(b, ".") {


I don’t like this check, just check To4() != nil inside the for-loop.

tmthrgd · 2020-09-27T10:21:39Z

svcb.go

+	var str strings.Builder
+	str.Grow(4 * len(s.Data))
+	for _, e := range s.Data {
+		if (0x1f < e && e < 0x7f) || e == 0x09 {


I’d still like this to be clearer.

svcb.go

DesWurstes · 2020-09-27T13:03:40Z

Just to confirm the wire format is also base64 encoded?

Thank you, this used to be a broken implementation. I've now verified the correctness of its behavior with ghedo's Python tool.

I don’t love this check. Is there any harm in letting people use IPv4-in-IPv6 addresses?

This was to be consistent with #1107, unless something changed in the last few months?

tmthrgd · 2020-09-27T13:22:39Z

@DesWurstes

“This was to be consistent with #1107, unless something changed in the last few months?”

My memory apparently, months is a life time ago. 😅

If it’s consistent with elsewhere in this library, it’s fine to stay.

This branch exists to have pull 1067 changes on top of most recent upstream miekg/master branch. This branch took DesWurstes's master branch commit that diverged from commit upstream commit 0ffcea3 and continued to 5cfc4ab. The last commit 5cfc4ab... had title 'More cleanup and fix mandatory not sorting bug'. Reference: miekg#1067 Reference: https://github.com/DesWurstes/dns.git

tmthrgd

I'm happy with this now, no need to iterate ad infinitum over minor nits. Should be fine to merge. Thanks for your patience.

* Rename ECH, bump draft number * AliasForm new treatment * alpn is no longer mandatory by default * Document the non-empty value requirement * new test cases * more test cases * Continue forbidding v4-map-v6 but not v4-embed-v6 #1067 (comment) and MikeBishop/dns-alt-svc#361 * Update documentation * revert rename ech * Reword AliasMode with key=value pairs

@ghedo

* Implement SVCB * Fix serialization and deserialization of double quotes * More effort (?) 4 months old commit * DEBUG * _ * Presentation format serialization/deserialization * _ Remove generated * Progress on presentation format parse & write * _ * Finish parsing presentation format * Regenerate * Pack unpack * Move to svcb.go Scan_rr.go and types.go should be untouched now * 🐛 Thanks ghedo * Definitions * TypeHTTPSSVC * Generated and isDuplicate * Goodbye lenient functions Now private key=value pairs have to be defined as structs too. They are no longer automatically named as KeyNNNNN * Encode/decode * Experimental svc * Read method * Implement some of the methods, use trick... to report where the error is while reading it. This should be applied to EDNS too. Todo: Find if case can only contain e := new(SVC_ALPN) and rest moved out Also fix two compile errors * Add SVC_LOCAL methods, reorder, remove alpn value, bugs * Errors * Alpn, make it build * Correct testsuite * Fully implement parser Change from keeping a state variable to reading in one iteration until the key=value pair is fully consumed * Simplify and document EDNS should be simplified too * Attempt to fix fuzzer And Alpn bug * A bug and change type values to match @ghedo's implementation * IP bug Also there are two ip duplicating patterns, one with copy, one with append. Maybe change it to be consistent. * Check for strictly increasing keys as required * Don't panic on invalid alpn * Redundant check, don't modify original array * Size calculation * Fix the fuzzer, match the style * 65535 is reserved too, don't delay errors * Check keyNNN, check for aliasform having values * IPvNHint is an array * Fix ipvNHint * Rename everything * Unrecognized keys according to the updated specification * Skip zero-length structs in generators. Fix CI * Doc cleanup * Off by one * Add parse tests * Check if private key doesn't collide with known key, invalid tests * Disallow IPv4 as IPv6. More tests. Related miekg#1107 * Style fixes * More consistency, more tests * 🐛 Deep copy as in the documentation a := make([]net.IP, 1) a[0] = net.ParseIP("1.1.1.1").To4() b := append(make([]net.IP, 0, 1), a...) b[0] = net.ParseIP("3.1.1.1").To4() fmt.Println(a[0][0]) * Make tests readable * Move valid parse tests to different file * 🐛 One of previous commits not fully committed * Test binary single value encoding/decoding and full encode/decode * Add worst-case grows to builders, 🐛 Wrong visible character range, redundant tests * Testing improvements And don't convert to IPv4 twice * Doc update only * Document worst case allocations and ipv6 can be at most of length 39, not 40 * Redundant IP copy, consistent IPv6 behavior, fix deep copy * isDuplicate for SVCB * Optimizations * echoconfig * Svc => SVCB * Fix CI * Regenerate after REBASE (2) Rebased twice on 15th and 20th May * Rename svc, use escapeByte. * Fix parsing whitespaces between quotes, rename ECHOHOConfig * resolve Remove svcbFieldLen Use reverseInt Uppercase SVCB Rename key_value "invalid" => bad Alpn comments > 65535 check Unneeded slices * a little more read => parse IP array meaning Force pushed because forgot to change read in svcb_test.go * HTTPSSVC -> HTTPS * Use new values * mandatory code MikeBishop/dns-alt-svc#205 * Resolve comments Rename svcb-pairs Remove SVCB_PRIVATE ranges Comment on SVCB_KEY65535 ParseError return l.token rename svcbKeyToString and svcbStringToKey privatize SVCBKeyToString, SVCBStringToKey * Refactor 1 Rename sorted, originalPairs Use append instead of copy Use svcb_RESERVED instead of 65535, with it now being private "type SVCBKey uint16" * Refactor 2 svcbKeyToString as method svcbStringToKey updated after key 0 :bug: mandatory has missing key Rename str idx < 0 * Refactor 3 Use l.token as z var key, value string Comment wrap 0: Sentences with '.' keyValue => kv * Refactor 4 * Refactor 5 len() int * Refactor 6 * Refactor 7 * Test remove parsing * Error messages * Rewrite two estimate comments * parse shouldn't modify original array :bug: * Remove two unneeded comments * Address review comments Push 2 because can't build fuzzer python Push 3 to try again * Simplify argument duplication as per tmthrgd's suggestion And add the relevant test Force push edit: Make sorting code fit into one line * Rewrite ECHConfig and address the review * Remove the optional tab * Add To4() Check * More cleanup and fix mandatory not sorting bug

* Rename ECH, bump draft number * AliasForm new treatment * alpn is no longer mandatory by default * Document the non-empty value requirement * new test cases * more test cases * Continue forbidding v4-map-v6 but not v4-embed-v6 miekg#1067 (comment) and MikeBishop/dns-alt-svc#361 * Update documentation * revert rename ech * Reword AliasMode with key=value pairs

DesWurstes requested review from miekg and tmthrgd as code owners December 30, 2019 07:39

miekg requested changes Dec 30, 2019

View reviewed changes

scan_rr.go Outdated Show resolved Hide resolved

types.go Outdated Show resolved Hide resolved

ghedo reviewed Apr 17, 2020

View reviewed changes

DesWurstes mentioned this pull request Apr 24, 2020

Clarify serialization of values of unknown SvcParamKeys MikeBishop/dns-alt-svc#140

Merged

miekg reviewed Apr 28, 2020

View reviewed changes

DesWurstes added 3 commits July 26, 2020 10:49

Refactor 6

ea0812c

Refactor 7

278d221

Test remove parsing

6976c49

DesWurstes added 4 commits July 26, 2020 11:46

Error messages

133d88c

Rewrite two estimate comments

6b92114

parse shouldn't modify original array 🐛

2ca90e6

Remove two unneeded comments

6b636a5

tmthrgd requested changes Aug 5, 2020

View reviewed changes

Address review comments

7c0f78e

Push 2 because can't build fuzzer python Push 3 to try again

jedisct1 mentioned this pull request Sep 12, 2020

dns TYPE65 DNSCrypt/dnscrypt-proxy#1473

Closed

Simplify argument duplication as per tmthrgd's suggestion

297d02d

And add the relevant test Force push edit: Make sorting code fit into one line

tmthrgd requested changes Sep 27, 2020

View reviewed changes

DesWurstes added 2 commits September 27, 2020 13:57

Rewrite ECHConfig and address the review

40bca2d

Remove the optional tab

dae3391

DesWurstes added 2 commits September 27, 2020 19:00

Add To4() Check

59c9584

More cleanup and fix mandatory not sorting bug

5cfc4ab

tmthrgd approved these changes Oct 11, 2020

View reviewed changes

miekg merged commit 0972db6 into miekg:master Oct 11, 2020

chris-wood mentioned this pull request Dec 12, 2020

Add support for SVCB and HTTPS resource record queries zmap/zdns#236

Closed

tanyav2 mentioned this pull request Dec 17, 2020

Support for SVCB/HTTPS record type hickory-dns/hickory-dns#1323

Closed

DesWurstes mentioned this pull request Feb 20, 2022

Update SVCB #1341

Merged

Implement SVCB #1067

Implement SVCB #1067

Conversation

DesWurstes commented Dec 30, 2019 • edited Loading

codecov-io commented Dec 30, 2019 • edited Loading

Codecov Report

miekg commented Dec 30, 2019 via email

miekg commented Dec 30, 2019 via email

DesWurstes commented Dec 30, 2019

miekg commented Dec 30, 2019 via email

DesWurstes commented Dec 31, 2019

miekg commented Dec 31, 2019 via email

ghedo commented Apr 2, 2020

DesWurstes commented Apr 2, 2020

DesWurstes commented Apr 3, 2020 • edited Loading

miekg commented Apr 5, 2020 via email

DesWurstes commented Apr 9, 2020 • edited Loading

miekg commented Apr 10, 2020 via email

DesWurstes commented Apr 10, 2020 • edited Loading

miekg commented Apr 10, 2020 via email

DesWurstes commented Apr 16, 2020

ghedo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghedo commented Apr 17, 2020

DesWurstes commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghedo commented May 1, 2020

DesWurstes commented May 1, 2020 • edited Loading

miekg commented May 6, 2020 via email

miekg commented May 6, 2020 via email

DesWurstes commented May 6, 2020

DesWurstes commented Jul 26, 2020 • edited Loading

tmthrgd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DesWurstes commented Aug 6, 2020

ghedo commented Sep 18, 2020

tmthrgd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DesWurstes commented Sep 27, 2020 • edited Loading

tmthrgd commented Sep 27, 2020

tmthrgd left a comment

Choose a reason for hiding this comment

DesWurstes commented Dec 30, 2019 •

edited

Loading

codecov-io commented Dec 30, 2019 •

edited

Loading

DesWurstes commented Apr 3, 2020 •

edited

Loading

DesWurstes commented Apr 9, 2020 •

edited

Loading

DesWurstes commented Apr 10, 2020 •

edited

Loading

DesWurstes commented May 1, 2020 •

edited

Loading

DesWurstes commented Jul 26, 2020 •

edited

Loading

DesWurstes commented Sep 27, 2020 •

edited

Loading